Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jul 9, 2019

  • Dont remove all chat notifications when pulling for new messages
  • Also notifier users which are active in the chat
  • Remove all chat notifications when joining a room
  • Mark chat notifications of read messages done

Fix #1575

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 tested, works

Tested by having read and unread messages and I could see the notifications either stay (when unread in the message list) or disappear (when read in the message list). Of course had to wait for notifications to be pulled.

Also confirmed that notifications get cleared when leaving a room.

@nickvergessen
Copy link
Member Author

Also confirmed that notifications get cleared when leaving a room.

It shouldn't but I guess it does because of the readmarker?

Copy link
Member

@Ivansss Ivansss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs re-thinking.
Always sending push notifications even when the user is active in a mobile app will generate a weird user experience.

@PVince81
Copy link
Member

Also confirmed that notifications get cleared when leaving a room.

It shouldn't but I guess it does because of the readmarker?

Possibly. But I think this behavior is acceptable.
I actually wanted to try the item you listed where you clear notifications when joining. So even if notifications are kept after leaving, if you'd rejoin an open conversation, it would clear them anyway.

@ASerbinski
Copy link

I've been testing these changes, and there is one fairly minor piece missing; when a message is specifically marked as read, it doesn't actually delete the notification. It is necessary to actually exit the conversation and re-enter it in order for the notification(s) to be deleted.

For example to reproduce;

Text chat conversation is open between two users in browsers, User1 sends a message to User2. User1 sees the "single check" mark that the message has been sent, User2 receives a notification (possibly on phone) and the message appears at the bottom of the chat window. User2 scrolls down to read the message, marking it as read, User1 now sees the "double check" mark indicating that the message has been read. The notification received by User2 has NOT been dismissed -- it remains.

I believe that the event of marking the message actually as read should also trigger the dismissal of the notification.

Aside from that, this seems perfect to me. I especially like how mobile notifications continue even if the conversation is open in the browser.

@nickvergessen
Copy link
Member Author

That's a different issue: #5829
I could only notice it in one-to-one chats yet, but have no fixed reproduction steps which work reliably to reproduce and fix the issue

@ASerbinski
Copy link

That's a different issue: #5829 I could only notice it in one-to-one chats yet, but have no fixed reproduction steps which work reliably to reproduce and fix the issue

No, that is not correct. It worked before but was broken by this commit: 6838a7f

@nickvergessen
Copy link
Member Author

But that commit is not merged, this Pull Request is still open :P

@ASerbinski
Copy link

But that commit is not merged, this Pull Request is still open :P

Yes, that commit is part of THIS PR, which is obviously why I am commenting on it here.

@theoneandonly-vector
Copy link

nickvergessen added this to the 💚 Next Major milestone on 9 Jul 2019
any ETA?

@Smith-fms
Copy link

Needs re-thinking. Always sending push notifications even when the user is active in a mobile app will generate a weird user experience.

I confirm, its annoying.... There really is room for improvement.

@theoneandonly-vector
Copy link

Getting the message everywhere is better than nowhere

@DaleJP
Copy link

DaleJP commented Nov 29, 2021

I'm really sorry to muddy this thread with something everybody probably knows, but how do I apply this fix? Or at least if someone can give me the search terms to find how to do it. I've been told this could fix my notification problems with Nextcloud Talk and I can't see an obvious way to install it.
I am brand new to github other than for looking at the readme for installation.

EDIT; Ok, so I've found the Files Changed section. Do I just hunt down the files and edit them with something like Nano, inserting the amended code, or is there a better way?

@Marhy172
Copy link

Hello @DaleJP,
I used this:

https://docs.nextcloud.com/server/latest/admin_manual/issues/applying_patch.html

I had an error while applying this patch. Some file not found or something like that, but it seems it works...

@DaleJP
Copy link

DaleJP commented Dec 1, 2021

Thanks @Marhy172 ,

I'm stuck at the instruction "download the patch" and even then, I'm not 100% sure in which directory to apply the patch when I have the file.
Do I download using wget https://github.com/nextcloud/spreed/pull/1970.patch to /var/www/nextcloud/spreed/?
Then, do I apply the fix using patch -p 1 < /var/www/nextcloud/spreed/1970.patch"?

@Marhy172
Copy link

Marhy172 commented Dec 1, 2021

First of all, I am amateur myself. I use Nextcloud on Raspberry Pi to learn more about internet, networks, Linux, etc. I use dd to backup whole system whenever I make changes I don't fully understand. So consider yourself if it worth to you to follow my steps :) I did it like this:

  • downloaded patch to home folder using wget as you (for example /my/home/folder)
  • went to root folder of spreed app (probably cd /var/www/nextcloud/apps/spreed/ in yours case?)
  • applied downloaded patch from my home folder: patch -p 1 < /my/home/folder/1970.patch

As I written. I had an error while applying the patch like this. So I am not sure if this is the right way to do it for this patch. But it seems notifications works for me and my friends now. Maybe wait if someone more educated answers you :)

@DaleJP
Copy link

DaleJP commented Dec 1, 2021

Thanks again @Marhy172 ,
It seems to have worked! It errored and then asked me which file to patch. I skipped that part and it patched a couple of other files. Notifications are now working for the moment!

Thank you so much for your help. I had no idea that patches could work this way.

@nickvergessen nickvergessen deleted the bugfix/1575/better-chat-notification-handling branch March 22, 2022 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notifications for chats are not showing when browser tab/window is inactive

9 participants